[Bugfix] Fix JWT Secret Tail characters #1867
Open
+794
−209
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jwierzbo
approved these changes
Apr 9, 2025
ajanikow
force-pushed
the
bugfix/fix_secret_discovery
branch
2 times, most recently
from
bc9ded2 to
fcdfb59
Compare
ajanikow
force-pushed
the
bugfix/fix_secret_discovery
branch
from
fcdfb59 to
693c92c
Compare
ajanikow
force-pushed
the
bugfix/fix_secret_discovery
branch
5 times, most recently
from
315ccaa to
0209639
Compare
ajanikow
force-pushed
the
bugfix/fix_secret_discovery
branch
from
701bb68 to
c19a0ef
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the JWT secret handling to use a fixed-size Secret abstraction, adds trimming/filling of secret byte slices, and replaces direct JWT calls with a unified Secret/Claims API throughout the codebase.
- Introduce
Secretinterface and implementations (secret,Secrets,emptySecret,secretSet) with trimming/filling toDefaultTokenSecretSize - Update token parsing/signing in
pkg/util/tokenand callers to useClaims.SignandSecret.Validate - Add extensive tests in
token_test.gofor trimming, filling, and prefix/postfix removal behaviors
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/token/token.go | New Validate and token type wrapping with fixed-size secrets |
| pkg/util/token/secret.go | Trim/fill logic for secrets, NewSecretWithSize implementation |
| pkg/util/token/secrets.go | Secrets collection type and validation over a set |
| pkg/util/token/secret_set.go | secretSet combining primary and secondary secrets |
| pkg/util/token/secret_empty.go | Empty secret stub |
| pkg/util/token/mods.go | Updated modifiers signature to util.ModR |
| pkg/util/token/claims.go | Claims builder and .Sign method |
| pkg/util/token/interface.go | Introduced Secret and Token interfaces |
| pkg/util/token/consts.go | JWT claim constants |
| pkg/util/token/method.go | Default JWT signing method |
| pkg/util/token/token_test.go | New tests covering trimming, filling, prefix/postfix behavior |
| pkg/util/mod.go | Moved emptyModR and updated ApplyModsR |
| pkg/util/k8sutil/secrets.go | Added GetTokenSecret overloads |
| Multiple callers (k8sutil, arangod, backup handlers, deployment resources, CLI, integration) | Switched to Claims.Sign and Secret.Validate usage |
Comments suppressed due to low confidence (1)
pkg/util/token/token_test.go:207
- [nitpick] The test name "Ensure token gets trimmed" is used twice; consider renaming one of them to clearly distinguish the scenarios.
t.Run("Ensure token gets trimmed", func(t *testing.T) {
| ) | ||
|
|
||
| type Mod func(in Claims) Claims | ||
| func Validate(t string, secret []byte) (Token, error) { |
There was a problem hiding this comment.
Enforce the expected signing algorithm in Validate by adding jwt.WithValidMethods(jwt.SigningMethodHS256.Alg()) (or a similar option) to jwt.Parse to avoid accepting tokens signed with none or other algorithms.
Copilot uses AI. Check for mistakes.
|
|
||
| return !equality.Semantic.DeepDerivative(tokenClaims, exporterTokenClaims), true, nil | ||
| return !equality.Semantic.DeepDerivative(tokenClaims, expectedClaims), true, nil |
There was a problem hiding this comment.
Pass the actual claims map, not the Token interface, to DeepDerivative. For example:
return !equality.Semantic.DeepDerivative(tokenClaims.Claims(), expectedClaims), true, nil
Suggested change
| return !equality.Semantic.DeepDerivative(tokenClaims, expectedClaims), true, nil | |
| return !equality.Semantic.DeepDerivative(tokenClaims.Claims(), expectedClaims), true, nil |
Copilot uses AI. Check for mistakes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.